-
Notifications
You must be signed in to change notification settings - Fork 327
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PLAT-931] Retrieve/Delete User External Identities in APIv2 #8540
[PLAT-931] Retrieve/Delete User External Identities in APIv2 #8540
Conversation
… into get-delete-external-ids * 'master' of https://github.com/CenterForOpenScience/osf.io: (27 commits) Update entity id / login url for IIT Add institution Illinois Institute of Technology (IIT) Update CHANGELOG, package.json Allow "upload" action to registrations being archived Fix institutions order in `scripts/populate_institutions.py` Add new institution CMU to test server only Fix None subjects bug Add 'provider' to ES CGM docs fix prob fail for TestNodeDetail.test_node_shows_wiki_relationship_based_on_disabled_status_and_version Add SearchParser - update perms and views Add test for expected search POST behavior Remove deprecated kwarg -Breaking change from "master" Update changed method signatures -Broke with "master" changes Add merge migration Remove duplicative Addr/Privacy info -Already in base template -Also fix <br/> tag Add form validation for asset naming conflicts Include comment in "edit-comment" email Add Choices to ProviderAssetFile.name Add merge migration Prevent outgoing blob requests when getting URLs ...
into get-delete-external-ids * 'develop' of https://github.com/CenterForOpenScience/osf.io: Integrate django-silk for profiling avoid failure on coveralls client add code coverage to tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Johnetordoff, several changes requested, I went ahead and made a PR to your branch Johnetordoff#5 since I'd tested a lot of this myself locally. That has a lot of what I mentioned here, except for tests.
api/users/views.py
Outdated
@@ -436,3 +437,66 @@ def perform_destroy(self, instance): | |||
if val['id'] in current_institutions: | |||
user.remove_institution(val['id']) | |||
user.save() | |||
|
|||
|
|||
class UserIdentitiesList(JSONAPIBaseView, generics.RetrieveAPIView, UserMixin): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List views should return a queryset(ideally) or a list of items. In our case, we have to return a list of user identities since this information is stored as a dict on the User. I would inherit from the ListAPIView
, and override get_queryset
to build the list of items. You are attempting to format the user identities that the user would see under get_object
, but this is the serializer's job. Just return the items you want to serialize here.
My recommendation:
class UserIdentitiesList(JSONAPIBaseView, generics.ListAPIView, UserMixin):
permission_classes = (
base_permissions.TokenHasScope,
drf_permissions.IsAuthenticatedOrReadOnly,
CurrentUser,
)
serializer_class = UserIdentitiesSerializer
required_read_scopes = [CoreScopes.USERS_READ]
required_write_scopes = [CoreScopes.NULL]
view_category = 'users'
view_name = 'user-identities-list'
# overrides ListAPIView
def get_queryset(self):
user = self.get_user()
identities = []
for key, value in user.external_identity.iteritems():
identities.append({'_id': key, 'external_id': value.keys()[0], 'status': value.values()[0]})
return identities
view_category = 'users' | ||
view_name = 'user-identities-detail' | ||
|
||
def get_object(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comments to UserIdentitiesList, get_object
should return the object you're trying to serialize, in this case, the individual external identity, rather than attempting to format the presentation of the data.
api/users/serializers.py
Outdated
@@ -252,3 +252,17 @@ def get_absolute_url(self, obj): | |||
|
|||
class Meta: | |||
type_ = 'institutions' | |||
|
|||
|
|||
class UserIdentitiesSerializer(BaseAPISerializer): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We follow the JSONAPI specification to format our API responses, and your response here does not follow the spec. (The ticket was a little confusing here, and I can see why it made you think the data should be formatted this way). If you inherit from the JSONAPISerializer
instead of BaseAPISerializer
a lot of this is taken care of for you.
My recommendation, provided you change your views as well:
class UserIdentitiesSerializer(JSONAPISerializer):
id = IDField(source='_id', read_only=True)
type = TypeField()
external_id = ser.CharField(read_only=True)
status = ser.CharField(read_only=True)
links = LinksField({
'self': 'get_absolute_url',
})
def get_absolute_url(self, obj):
return absolute_reverse(
'users:user-identities-detail',
kwargs={
'user_id': self.context['request'].parser_context['kwargs']['user_id'],
'version': self.context['request'].parser_context['kwargs']['version'],
'identity_id': obj['_id']
}
)
class Meta:
type_ = 'external-identities'
api/users/serializers.py
Outdated
|
||
|
||
class UserIdentitiesSerializer(BaseAPISerializer): | ||
data = ser.DictField() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of defining a data
DictField, inherit from the JSONAPISerializer which will automatically place your information inside of a data dict. In this serializer you should define the individual fields you want to display for each identity, which are id
, type
, external_id
, and status
. If you are returning a list of all identities, the JSONAPISerializer takes care of returning a list of dictionaries. If you request a single identity, the serializer returns a data dictionary.
api/users/serializers.py
Outdated
data = ser.DictField() | ||
links = LinksField({'self': 'get_self_url'}) | ||
|
||
def get_self_url(self, obj): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The self
url we want to return here should be the detail url, not the list url.
assert res.content_type == 'application/vnd.api+json' | ||
assert res.json['data'] == {'ORCID': {'0000-0001-9143-4653': 'VERIFIED'}} | ||
|
||
def test_anonymous_gets_401(self, app, url): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a permission test for someone trying to access someone else's identity. Also a test where the identify doesn't exist that should 404.
assert res.status_code == 401 | ||
assert res.content_type == 'application/vnd.api+json' | ||
|
||
def test_authorized_delete_204(self, app, user, url): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test unauthorized delete as well. I would also add a couple of tests trying to use unauthorized request methods, such as trying to update an identity which should 405.
api/users/serializers.py
Outdated
}) | ||
|
||
class Meta: | ||
type_ = 'external_identities' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In ticket, type is specified as needing a hyphen instead of an underscore. Looks like we are inconsistent as to the format here in other serializers though.
permission_classes = ( | ||
base_permissions.TokenHasScope, | ||
drf_permissions.IsAuthenticatedOrReadOnly, | ||
CurrentUser, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Yes, only logged in user should see their identities
api/users/views.py
Outdated
|
||
serializer_class = UserIdentitiesSerializer | ||
|
||
required_read_scopes = [CoreScopes.USERS_READ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if we should define more specific scopes for user settings, i.e. CoreScopes.USER_SETTINGS_READ, CoreScopes.USER_SETTINGS_WRITE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I'll plan to merge #8527 first so the CoreScopes added in that PR can be used here.
ad0be16
to
70441aa
Compare
Pull Request Test Coverage Report for Build 30557
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple little things @Johnetordoff !
|
||
def test_authorized_gets_200(self, app, user, url): | ||
res = app.get(url, auth=user.auth) | ||
print res.json['data'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove print statement.
print res.json['data'] | ||
assert res.status_code == 200 | ||
assert res.content_type == 'application/vnd.api+json' | ||
assert res.json['data'][0]['attributes'] == {u'status': u'LINK', u'external_id': u'0000-0001-9143-4652'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could define data = res.json['data']. Also, for all these tests, I prefer comparing individual attributes instead of comparing dictionaries for readability.
assert data[0]['attributes']['status'] == 'LINK'
|
||
|
||
@pytest.mark.django_db | ||
class TestUserIdentitiesDetail: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you still add the tests for an identity that 404's and a patch test that 405's? I know that this code works, but it prevents someone down the line from inadvertently inheriting from a new generic view or something and no one catches this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
res = app.get(url, auth=user.auth) | ||
assert res.status_code == 200 | ||
assert res.content_type == 'application/vnd.api+json' | ||
assert res.json['data'] == { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer comparing individual attributes instead of whole dictionaries for readability.
773dbe1
to
fc8a883
Compare
Thanks for the new tests @Johnetordoff. Looks good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the smallest of changes requested. Looks good, @Johnetordoff!
assert res.json['data']['attributes']['status'] == 'VERIFIED' | ||
assert res.json['data']['attributes']['external_id'] == '0000-0001-9143-4653' | ||
assert res.json['data']['type'] == 'external-identities' | ||
assert res.json['data']['links']['self'] == 'http://localhost:8000/v2/users/{}/settings/identities/ORCID/'.format(user._id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you change this to assert that /v2/users/{}/settings/identities/ORCID/
is in the self link? We don't want tests to be dependent on a specific port.
api/users/views.py
Outdated
|
||
serializer_class = UserIdentitiesSerializer | ||
|
||
required_read_scopes = [CoreScopes.USERS_READ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I'll plan to merge #8527 first so the CoreScopes added in that PR can be used here.
api/users/views.py
Outdated
|
||
class UserIdentitiesList(JSONAPIBaseView, generics.ListAPIView, UserMixin): | ||
""" | ||
REMEMBER TO WRITE DEV DOCS!!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation is good to go. Go ahead and document 👍
into get-delete-external-ids * 'develop' of https://github.com/CenterForOpenScience/osf.io: (98 commits) Bump version and update changelog fix tests for non-dict based client attributes Update gitlab serializers for fangorn config js Add enforce_csrf waffle switch so that this can be merged earlier Update markdown-it-video with infinite loop fix Fix indentation Improve tests by using user cookie method and faster user creation Tests to ensure API requests with Session auth fail without CSRF protection Travis and Test Speed ups Rename --quick to --all Don't transact to avoid deadlocks Add script to fix poor mapping logic -h/t @caseyrollins Add same enforce_csrf method as drf's SessionAuthentication replace bad reverse migration with warning Use timezone.now() when checking for failed registrations Remove duplicate psyarxiv custom subject. Use fork of celery 4.2.1 with celery/celery#4839 merged Upgrade celery and kombu Set TIME_ZONE in settings add scheme to match UI ...
… into get-delete-external-ids * 'master' of https://github.com/CenterForOpenScience/osf.io:
into get-delete-external-ids * 'develop' of https://github.com/CenterForOpenScience/osf.io: fix mocking for gitlab
@Johnetordoff, can you go ahead and get this PR up to date with develop and use the |
into get-delete-external-ids * 'develop' of https://github.com/CenterForOpenScience/osf.io: Allow removing stuck registrations with addons Add write scopes instead of read scopes to write-only user endpoints. Add USER_SETTINGS scopes, mock send_email in throttling tests, and move user fixtures out so they can be shared between tests. Handle account export and deactivation requests in APIv2. Signed-off-by: John Tordoff <john@cos.io> # Conflicts: # api/users/serializers.py # api/users/urls.py # api/users/views.py
Signed-off-by: John Tordoff <john@cos.io>
api/users/views.py
Outdated
serializer_class = UserIdentitiesSerializer | ||
|
||
required_read_scopes = [CoreScopes.USER_SETTINGS_READ] | ||
required_write_scopes = [CoreScopes.USER_SETTINGS_WRITE] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be CoreScopes.NULL
since this endpoint is read-only
Signed-off-by: John Tordoff <john@cos.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one non-blocking nit. This is good to merge.
user = self.get_user() | ||
identities = [] | ||
for key, value in user.external_identity.iteritems(): | ||
identities.append({'_id': key, 'external_id': value.keys()[0], 'status': value.values()[0]}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: You can use a list comprehension to build identities
.
Purpose
As part of out on going Emberification, we our exposing the external identity information to our V2 API.
Changes
QA Notes
http://localhost:8000/v2/users/<user_id>/settings/identities/
check to make sure it's coolhttp://localhost:8000/v2/users/<user_id>/settings/identities/<identity_id>
likehttp://localhost:8000/v2/users/<user_id>/settings/identities/ORCID/
is probably the only valid one for now.http://localhost:8000/v2/users/<user_id>/settings/identities/<identity_id>
make sure its is gone.Documentation
Docs will be add when this gets approved.
Side Effects
None that I know of.
Ticket
https://openscience.atlassian.net/projects/PLAT/issues/PLAT-931